Driver: Avoid llvm::sys::path::append if resource directory absolute. #146449
Conversation
Created using spr 1.3.6-beta.1
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-support Author: Peter Collingbourne (pcc) Changesllvm::sys::path::append does not append absolute paths in the way Many tests start failing if I try to align llvm::sys::path::append with Full diff: https://github.com/llvm/llvm-project/pull/146449.diff 2 Files Affected:
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 775f3f029861c..d86f47bada86b 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -192,7 +192,12 @@ std::string Driver::GetResourcesPath(StringRef BinaryPath) {
StringRef ConfiguredResourceDir(CLANG_RESOURCE_DIR);
if (!ConfiguredResourceDir.empty()) {
- llvm::sys::path::append(P, ConfiguredResourceDir);
+ // FIXME: We should fix the behavior of llvm::sys::path::append so we don't
+ // need to check for absolute paths here.
+ if (llvm::sys::path::is_absolute(ConfiguredResourceDir))
+ P = ConfiguredResourceDir;
+ else
+ llvm::sys::path::append(P, ConfiguredResourceDir);
} else {
// On Windows, libclang.dll is in bin/.
// On non-Windows, libclang.so/.dylib is in lib/.
diff --git a/llvm/include/llvm/Support/Path.h b/llvm/include/llvm/Support/Path.h
index 0d8881359b806..ee8b1779b68ad 100644
--- a/llvm/include/llvm/Support/Path.h
+++ b/llvm/include/llvm/Support/Path.h
@@ -223,6 +223,7 @@ LLVM_ABI bool remove_dots(SmallVectorImpl<char> &path,
/// /foo + bar/f => /foo/bar/f
/// /foo/ + bar/f => /foo/bar/f
/// foo + bar/f => foo/bar/f
+/// foo + /bar/f => foo/bar/f (FIXME: will be changed to /bar/f to align with C++17 std::filesystem::path::append)
/// @endcode
///
/// @param path Set to \a path + \a component.
|
|
@llvm/pr-subscribers-clang-driver Author: Peter Collingbourne (pcc) Changesllvm::sys::path::append does not append absolute paths in the way Many tests start failing if I try to align llvm::sys::path::append with Full diff: https://github.com/llvm/llvm-project/pull/146449.diff 2 Files Affected:
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 775f3f029861c..d86f47bada86b 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -192,7 +192,12 @@ std::string Driver::GetResourcesPath(StringRef BinaryPath) {
StringRef ConfiguredResourceDir(CLANG_RESOURCE_DIR);
if (!ConfiguredResourceDir.empty()) {
- llvm::sys::path::append(P, ConfiguredResourceDir);
+ // FIXME: We should fix the behavior of llvm::sys::path::append so we don't
+ // need to check for absolute paths here.
+ if (llvm::sys::path::is_absolute(ConfiguredResourceDir))
+ P = ConfiguredResourceDir;
+ else
+ llvm::sys::path::append(P, ConfiguredResourceDir);
} else {
// On Windows, libclang.dll is in bin/.
// On non-Windows, libclang.so/.dylib is in lib/.
diff --git a/llvm/include/llvm/Support/Path.h b/llvm/include/llvm/Support/Path.h
index 0d8881359b806..ee8b1779b68ad 100644
--- a/llvm/include/llvm/Support/Path.h
+++ b/llvm/include/llvm/Support/Path.h
@@ -223,6 +223,7 @@ LLVM_ABI bool remove_dots(SmallVectorImpl<char> &path,
/// /foo + bar/f => /foo/bar/f
/// /foo/ + bar/f => /foo/bar/f
/// foo + bar/f => foo/bar/f
+/// foo + /bar/f => foo/bar/f (FIXME: will be changed to /bar/f to align with C++17 std::filesystem::path::append)
/// @endcode
///
/// @param path Set to \a path + \a component.
|
|
@llvm/pr-subscribers-clang Author: Peter Collingbourne (pcc) Changesllvm::sys::path::append does not append absolute paths in the way Many tests start failing if I try to align llvm::sys::path::append with Full diff: https://github.com/llvm/llvm-project/pull/146449.diff 2 Files Affected:
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 775f3f029861c..d86f47bada86b 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -192,7 +192,12 @@ std::string Driver::GetResourcesPath(StringRef BinaryPath) {
StringRef ConfiguredResourceDir(CLANG_RESOURCE_DIR);
if (!ConfiguredResourceDir.empty()) {
- llvm::sys::path::append(P, ConfiguredResourceDir);
+ // FIXME: We should fix the behavior of llvm::sys::path::append so we don't
+ // need to check for absolute paths here.
+ if (llvm::sys::path::is_absolute(ConfiguredResourceDir))
+ P = ConfiguredResourceDir;
+ else
+ llvm::sys::path::append(P, ConfiguredResourceDir);
} else {
// On Windows, libclang.dll is in bin/.
// On non-Windows, libclang.so/.dylib is in lib/.
diff --git a/llvm/include/llvm/Support/Path.h b/llvm/include/llvm/Support/Path.h
index 0d8881359b806..ee8b1779b68ad 100644
--- a/llvm/include/llvm/Support/Path.h
+++ b/llvm/include/llvm/Support/Path.h
@@ -223,6 +223,7 @@ LLVM_ABI bool remove_dots(SmallVectorImpl<char> &path,
/// /foo + bar/f => /foo/bar/f
/// /foo/ + bar/f => /foo/bar/f
/// foo + bar/f => foo/bar/f
+/// foo + /bar/f => foo/bar/f (FIXME: will be changed to /bar/f to align with C++17 std::filesystem::path::append)
/// @endcode
///
/// @param path Set to \a path + \a component.
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Created using spr 1.3.6-beta.1
pcc
deleted the
users/pcc/spr/driver-avoid-llvmsyspathappend-if-resource-directory-absolute
branch
…y absolute. After #145996 CLANG_RESOURCE_DIR can be an absolute path so we need to handle it correctly in the driver. llvm::sys::path::append does not append absolute paths in the way that I expected (or consistent with other similar APIs such as C++17 std::filesystem::path::append or Python os.path.join); instead, it effectively discards the leading / and appends the resulting relative path (e.g. append(P, "/bar") with P = "/foo" sets P to "/foo/bar"). Many tests start failing if I try to align llvm::sys::path::append with the other APIs because of callers that expect the existing behavior, so for now let's add a special case here for absolute resource paths, and document the behavior in Path.h. Reviewers: MaskRay Reviewed By: MaskRay Pull Request: llvm/llvm-project#146449
After #145996 CLANG_RESOURCE_DIR can be an absolute path so we need to
handle it correctly in the driver.
llvm::sys::path::append does not append absolute paths in the way
that I expected (or consistent with other similar APIs such as C++17
std::filesystem::path::append or Python os.path.join); instead, it
effectively discards the leading / and appends the resulting relative path
(e.g. append(P, "/bar") with P = "/foo" sets P to "/foo/bar").
Many tests start failing if I try to align llvm::sys::path::append with
the other APIs because of callers that expect the existing behavior,
so for now let's add a special case here for absolute resource paths,
and document the behavior in Path.h.